Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type checks for typed arrays and array buffers. #291

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xeioex
Copy link
Contributor

@xeioex xeioex commented May 8, 2024

No description provided.

@kasperisager
Copy link
Contributor

I just found myself in need of this as well, thanks! 🙌 It'd be great if the function could also take a JSTypedArrayEnum pointer and, if it's not NULL, write the type of the view when val is a typed array.

quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
@xeioex xeioex force-pushed the feat-check-array-types branch 3 times, most recently from 40a7281 to adc36c9 Compare May 9, 2024 19:28
@xeioex
Copy link
Contributor Author

xeioex commented May 9, 2024

I just found myself in need of this as well, thanks! 🙌 It'd be great if the function could also take a JSTypedArrayEnum pointer and, if it's not NULL, write the type of the view when val is a typed array.

I can do this as well, any objections from reviewers @saghul?

@saghul
Copy link
Contributor

saghul commented May 9, 2024

No objections from me, but I would handle that in a separate PR.

@chqrlie
Copy link
Collaborator

chqrlie commented May 9, 2024

🙌 It'd be great if the function could also take a JSTypedArrayEnum pointer and, if it's not NULL, write the type of the view when val is a typed array.

I would favor an API that retrieves the typed array type, length and data address. A generalized version of JS_GetUint8Array.

As a matter of fact, this would make JS_IsTypedArray redundant, so maybe we should just have this:

struct JSTypedArrayDescription {
    JSTypedArrayEnum type;
    size_t length;
    void *data;
};

// Return value is -1 for proxy errors, 0 if `obj` is not a typed array, 1 if it is a typed array.
// The structure pointed to by `desc` is filled on success unless `desc` is a null pointer.
int JS_GetTypedArray(JSContext *ctx, JSValueConst obj, struct JSTypedArrayDescription *desc);

@xeioex
Copy link
Contributor Author

xeioex commented May 9, 2024

OK, I have no objection. Will update.

@xeioex
Copy link
Contributor Author

xeioex commented May 9, 2024

@chqrlie

Return value is -1 for proxy errors

Do we still want to check for proxy case here, given that JS_GetTypedArrayBuffer does not do that?

quickjs.h Outdated Show resolved Hide resolved
quickjs.h Outdated Show resolved Hide resolved
quickjs.h Outdated Show resolved Hide resolved
xeioex added 2 commits May 9, 2024 17:11
 Unlike JS_GetArrayBuffer() it does not throw an exception
 if `val` is not an array buffer.
quickjs.h Show resolved Hide resolved
@xeioex
Copy link
Contributor Author

xeioex commented May 10, 2024

The change is approved, but not merged. Any plans to align this PR with #294?

@chqrlie
Copy link
Collaborator

chqrlie commented May 10, 2024

The change is approved, but not merged. Any plans to align this PR with #294?

Yes, we are discussing prototypes... Trying to cook an API in a hurry does not work. We have to let it simmer for a few days

@xeioex
Copy link
Contributor Author

xeioex commented May 10, 2024

Agree. I have no problem with it. Just let me know of your intentions, it is enough.

TooTallNate pushed a commit to TooTallNate/quickjs that referenced this pull request Jul 3, 2024
Retrieves the current time zone settings with GetTimeZoneInformation to calculate time zone offset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants